Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix custom resource icons in FileSystem #77932

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 6, 2023

Fixes #32706
Supersedes #73611

@KoBeWi KoBeWi added this to the 4.1 milestone Jun 6, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner June 6, 2023 22:10
editor/filesystem_dock.cpp Outdated Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the custom_resources_to_kill_performance_again_probably branch from 9a111ff to a52b220 Compare June 7, 2023 11:26
@KoBeWi KoBeWi force-pushed the custom_resources_to_kill_performance_again_probably branch from a52b220 to 8d85da0 Compare June 7, 2023 14:52
@YuriSizov YuriSizov modified the milestones: 4.1, 4.x Jun 12, 2023
@YuriSizov
Copy link
Contributor

Yeah, the branch name is very fitting 🙃 This approach is going to read and parse the file for each stored resources every time we need to figure out its icon. Even if this information doesn't change, we would still parse the file, at least partially, to fetch the type information.

This definitely needs some cache, and that's what reduz wanted as well, I believe. To utilize the editor file system somehow, and only update the information when the underlying data, like the type or the script, changes. This way we would only have to run this expensive operation on change rather than on fetch.

I believe that the PR this one is supposed to supersede was going this route. After a lot of discussion they've hooked into the preview generator process, since it already needs to run for a changed resource. Maybe it's not the best place to hook into, but at least it creates an accessible cache for us to use on the file tree refreshes.

So in the end, I would not merge this in its current state and look for a more optimal approach using some sort of cache.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 14, 2023

I think the icons can be cached locally, we already do this with scene tree (see #60789). I'll look into that, I thought get_class_icon() already uses it...

@KoBeWi KoBeWi force-pushed the custom_resources_to_kill_performance_again_probably branch from 8d85da0 to 476d200 Compare March 8, 2024 21:16
@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 8, 2024

Ok rebased and updated. I used EditorFileSystem's cache to get resource icon, but it has caveats:

  • it assumes the first dependency in the file is its Script
  • it checks whether that dependency is actually a Script (not sure how fast is that)

I added a temporary HashMap that stores cache for each script path, so it shouldn't be too slow, but testing appreciated. If it's slow, it will affect file tree updates.

EDIT:
Tested in a big project and it doesn't seem to be particularly slow. Rescan takes the most time as normally.

@KoBeWi KoBeWi force-pushed the custom_resources_to_kill_performance_again_probably branch from 476d200 to a62ccf3 Compare March 8, 2024 21:31
Comment on lines +199 to +202
const String &script_path = deps[0]; // Assuming the first dependency is a script.
if (script_path.is_empty() || !ClassDB::is_parent_class(ResourceLoader::get_resource_type(script_path), SNAME("Script"))) {
return String();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe loop through the dependencies for the first script?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be slower though 🤔
I'd change this only if it proves unreliable.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my comment, everything seems fine! I tested it and it works.

Thanks @KoBeWi !

@adamscott adamscott modified the milestones: 4.x, 4.3 Mar 14, 2024
@akien-mga akien-mga merged commit 5cf38f8 into godotengine:master Mar 14, 2024
16 checks passed
@KoBeWi KoBeWi deleted the custom_resources_to_kill_performance_again_probably branch March 14, 2024 21:37
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom resources don't use icon for file in filesystem
4 participants